Object oriented refactoring of fcType by michelebucelli · Pull Request #577 · SimVascular/svMultiPhysics

michelebucelli · 2026-06-28T22:07:46Z

Fixes #575.

Current situation

The class fcType is currently defined in ComMod.h, and its interface and that of related function make its use slightly more cumbersome and less seamless than it needs to be (see discussion in #575 for more details).

Release Notes

fcType renamed to FourierInterpolation, and its data members (now private) renamed with more explicit names.
FourierInterpolation implementation moved to dedicated files fourier_interpolation.{h,cpp}.
Construction of FourierInterpolation object is now done through static methods FourierInterpolation::from_time_series and FourierInterpolation::from_fourier_coefficients (while before it was done "manually" everywhere this was needed).
Static methods FourierInterpolation::from_time_series_file and FourierInterpolation::from_fourier_coefficients_file manage reading data from file (while before it was done "manually" everywhere this was needed).
Distribution of data in parallel is done with the (non-static) method FourierInterpolation::distribute.
Evaluation of the interpolation is done through the methods FourierInterpolation::value and FourierInterpolation::value_and_derivative.
Functions fft and ifft were tightly related to fcType (they worked on an fcType instance), and they have been merged into the methods FourierInterpolation class (for construction and evaluation, respectively).
The rest of the code has been updated to adhere to this new interface.

All this should make it a bit easier to add support for reading time-dependent data from file in new classes (which is part of the aims of #519).

Documentation

The class and its methods have been given detailed Doxygen documentation.

Testing

The unit test and the integration tests all pass with the new interface.

TODO before marking as ready

expand suite of FFT-related unit tests

Code of Conduct & Contributing Guidelines

I agree to follow the Code of Conduct and Contributing Guidelines.

…e and construction

…r clarity

codecov · 2026-06-28T23:44:34Z

Codecov Report

❌ Patch coverage is 79.14230% with 107 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.47%. Comparing base (9632608) to head (1bbbef1).

Files with missing lines	Patch %	Lines
Code/Source/solver/fourier_interpolation.cpp	74.91%	77 Missing ⚠️
...nitTests/math_tests/test_fourier_interpolation.cpp	90.47%	14 Missing ⚠️
Code/Source/solver/read_files.cpp	76.47%	8 Missing ⚠️
Code/Source/solver/Core/Exception.h	0.00%	6 Missing ⚠️
Code/Source/solver/set_bc.cpp	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
+ Coverage   69.44%   69.47%   +0.02%     
==========================================
  Files         181      183       +2     
  Lines       34072    34208     +136     
  Branches     5930     5932       +2     
==========================================
+ Hits        23662    23765     +103     
- Misses      10273    10306      +33     
  Partials      137      137

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

claude

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

…ents_file

michelebucelli · 2026-06-29T16:39:53Z

 // SPDX-FileCopyrightText: Copyright (c) Stanford University, The Regents of the University of California, and others.
 // SPDX-License-Identifier: BSD-3-Clause


In this PR, the functions fft and ifft have been moved from this file to fourier_interpolation.cpp, inside the new FourierInterpolation class. I think this makes sense, since those functions were tightly coupled to fcType.

However, this file also contains another function, igbc, which is now the only function left. The function is not related to Fourier transform, so it seems odd to keep the name of this file as it is.

I propose renaming it to igbc.cpp (and the header to igbc.h).

@michelebucelli You might could put igbc in utils.h,cpp or all_fun.h,cpp depending on I suppose if it is fun or not.

michelebucelli · 2026-06-29T16:59:34Z

+    // @todo[michelebucelli] This is consistent with the old code, but is
+    //   incorrect when use_ramp = true. In that case, the derivative should be
+    //   zero outside the interpolation interval [initial_time, initial_time +
+    //   period].
+    if (evaluate_derivative)
+      derivative[i] = linear_trend_slopes[i];


This was probably an unintended behavior of the old code.

When using the "ramp" option, the following function is computed (the implementation is formulated slightly different):

$$ v(t) = \begin{cases} y_0 & \text{if } t < t_0 \\ y_0 + (y_{N-1} - y_{0}) \frac{t - t_0}{t_{N-1} - t_0} & \text{if } t_0 \leq t \leq t_{N-1} \\ y_{N-1} & \text{if } t > t_{N-1} \end{cases} $$

For times outside of the inerpolation interval $[t_0, t_{N-1}]$, I think the derivative should be set to zero:

Suggested change

// @todo[michelebucelli] This is consistent with the old code, but is

// incorrect when use_ramp = true. In that case, the derivative should be

// zero outside the interpolation interval [initial_time, initial_time +

// period].

if (evaluate_derivative)

derivative[i] = linear_trend_slopes[i];

if (evaluate_derivative) {

if (use_ramp && (time < initial_time || time > initial_time + period)) {

derivative[i] = 0.0;

} else {

derivative[i] = linear_trend_slopes[i];

}

}

I tried this out and all the tests still pass, so I propose changing this behavior.

michelebucelli · 2026-06-29T17:53:34Z

+        // @todo[michelebucelli] This seems pretty fragile! It happens in a few
+        //   other places in this file as well, where using an increment
+        //   operator (*= or +=) changes the order of operations resulting in
+        //   changes in the test values. This should be investigated and the
+        //   best (i.e. most accurate) operation order should be chosen.
+        value[j] = value[j] + fourier_coefficients_real(j, i) * std::cos(K) -
+                   fourier_coefficients_imaginary(j, i) * std::sin(K);


This took me a while to figure out 😅 There's at least another place where the same thing happens. I'm honestly not sure which is the most correct option, but the fact that the order of operations makes a measurable difference probably means that this needs to be carefully evaluated.

@michelebucelli Very interesting !

What is the measurable difference that you are seeing ?

michelebucelli · 2026-06-29T18:01:58Z

+  std::tie(value, derivative) = gt.value_and_derivative(x_start - 1.0);
+  ASSERT_NEAR(value[0], temporal_values[0][1], 1e-6)
+      << "Expected value before initial time to be equal to initial value of "
+         "linear trend";
+
+  // Check that after the final time the value is equal to the final value of
+  // the linear trend.
+  std::tie(value, derivative) = gt.value_and_derivative(x_end + 1.0);
+  ASSERT_NEAR(value[0], temporal_values.back()[1], 1e-6)
+      << "Expected value after final time to be equal to final value of linear "
+         "trend";
+
+  // Check that the value in the interpolation interval is a linear
+  // interpolation of the initial and final value.
+  for (double t = x_start; t <= x_end; t += 0.5) {
+    std::tie(value, derivative) = gt.value_and_derivative(t);
+
+    const double expected_y =
+        temporal_values[0][1] +
+        (temporal_values.back()[1] - temporal_values[0][1]) * (t - x_start) /
+            (x_end - x_start);
+    const double expected_z =
+        temporal_values[0][2] +
+        (temporal_values.back()[2] - temporal_values[0][2]) * (t - x_start) /
+            (x_end - x_start);
+
+    ASSERT_NEAR(value[0], expected_y, 1e-6)
+        << "Expected value in interpolation interval to be linear "
+           "interpolation of initial and final value";
+    ASSERT_NEAR(value[1], expected_z, 1e-6)
+        << "Expected value in interpolation interval to be linear "
+           "interpolation of initial and final value";


These assertions should be extended to include checks on the evaluated derivative, once we agree whether to change or keep the current behavior as per my previous comment.

ktbolt

@michelebucelli Nice work !

We name files containing classes with their class name (I think).

ktbolt · 2026-06-29T23:47:52Z

 // SPDX-FileCopyrightText: Copyright (c) Stanford University, The Regents of the University of California, and others.
 // SPDX-License-Identifier: BSD-3-Clause


@michelebucelli You might could put igbc in utils.h,cpp or all_fun.h,cpp depending on I suppose if it is fun or not.

ktbolt · 2026-06-30T00:03:01Z

+  for (unsigned int j = 0; j < n_components; ++j) {
+    result.linear_trend_initial_values[j] = values(j, 0);
+    result.linear_trend_slopes[j] =
+        (values(j, n_time_points - 1) - values(j, 0)) / result.period;


@michelebucelli Should you check if result.period is zero ?

ktbolt · 2026-06-30T00:15:39Z

+
+      for (unsigned int j = 0; j < result.n_components; j++) {
+        const double s =
+            (values(j, i + 1) - values(j, i)) / (times[i + 1] - times[i]);


@michelebucelli Why all of these const ? Maybe a (good?) habit to guard against changing these variables later I guess.

ktbolt · 2026-06-30T00:18:01Z

+
+    for (unsigned int i = 0; i < n_time_points - 1; ++i) {
+      const double ko = 2.0 * consts::pi * tmp * times[i] / result.period;
+      const double kn = 2.0 * consts::pi * tmp * times[i + 1] / result.period;


@michelebucelli Why are we using consts::pi here rather than std::numbers::pi ?

ktbolt · 2026-06-30T00:27:22Z

+FourierInterpolation
+FourierInterpolation::from_time_series_file(const std::string &file_name,
+                                            const unsigned int n_components,
+                                            const bool use_ramp) {


@michelebucelli Fundamental types are typically not declared const.

ktbolt · 2026-06-30T00:35:27Z

+        "At least 1 Fourier coefficient is required to set up Fourier "
+        "interpolation. 0 Fourier coefficients were provided.");
+  }
+


@michelebucelli n_time_points could be zero for an incorrect format file format: first value is non-numeric.

And n_fourier_coefficients could be zero if the first value is not an int or there is a format error, say a comma is used to separate values.

ktbolt · 2026-06-30T00:43:21Z

+  bool initialized = defined();
+  cm.bcast(cm_mod, &initialized);
+
+  if (initialized) {


@michelebucelli It is not very clear what it means for an object to be initialized. Does that mean it has data ?

ktbolt · 2026-06-30T01:08:50Z

+        // @todo[michelebucelli] This seems pretty fragile! It happens in a few
+        //   other places in this file as well, where using an increment
+        //   operator (*= or +=) changes the order of operations resulting in
+        //   changes in the test values. This should be investigated and the
+        //   best (i.e. most accurate) operation order should be chosen.
+        value[j] = value[j] + fourier_coefficients_real(j, i) * std::cos(K) -
+                   fourier_coefficients_imaginary(j, i) * std::sin(K);


@michelebucelli Very interesting !

What is the measurable difference that you are seeing ?

michelebucelli added 12 commits June 24, 2026 16:31

Move fcType to separate header and source files

01279f6

Move evaluation of inverse Fourier transform to fcType methods

0fb860b

Distribution of fcType members encapsulated in fcType::distribute

ba5234c

Introduce getter method fcType::dimension

6f460ab

Move fft into fcType, and cleaner separation of fcType input from fil…

bc6813c

…e and construction

Make data members of fcType private

4740316

Add cmType::bcast overload for unsigned integers

a371f0a

Rename fcType to FourierInterpolation, and rename its data members fo…

e3f4436

…r clarity

Update and expand documentation of FourierInterpolation

03c2247

Use exceptions for correctness checks in FourierInterpolation

1dfbc49

Fix documentation and remove todo comment

219380e

Expand comment

5097084

michelebucelli requested review from dseyler, kko27 and ktbolt June 28, 2026 22:07

michelebucelli added 4 commits June 29, 2026 10:23

Replace leftover throw with svmp::raise in FourierInterpolation

f75f325

Remove duplicate unused file test_fft.h

8c53818

Fix typo in documentation

44a3f77

Update and expand unit tests for Fourier interpolation

8d5ec8d

michelebucelli marked this pull request as ready for review June 29, 2026 16:34

claude Bot reviewed Jun 29, 2026

View reviewed changes

michelebucelli added 3 commits June 29, 2026 12:57

Miscellaneous cleanup of FourierInterpolation

31262f0

Add correctness checks to FourierInterpolation::from_fourier_coeffici…

ae0d17d

…ents_file

Change for loop counter to unsigned int

1bbbef1

michelebucelli commented Jun 29, 2026

View reviewed changes

This was referenced Jun 29, 2026

Add scaffold surface support for URIS valves #569

Open

Object-oriented refactoring of active stress #578

Draft

michelebucelli added the OOP Refactor Object-Oriented Programming Refactor of Code label Jun 29, 2026

ktbolt reviewed Jun 30, 2026

View reviewed changes

		// SPDX-FileCopyrightText: Copyright (c) Stanford University, The Regents of the University of California, and others.
		// SPDX-License-Identifier: BSD-3-Clause

Uh oh!

Conversation

michelebucelli commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current situation

Release Notes

Documentation

Testing

TODO before marking as ready

Code of Conduct & Contributing Guidelines

Uh oh!

codecov Bot commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktbolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

michelebucelli commented Jun 28, 2026 •

edited

Loading

codecov Bot commented Jun 28, 2026 •

edited

Loading